-
Notifications
You must be signed in to change notification settings - Fork 786
[SYCL][NFCI] Drop __spirv_ops.hpp
from core.hpp
#18839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SYCL][NFCI] Drop __spirv_ops.hpp
from core.hpp
#18839
Conversation
Our device compiler have been capable of automagically declaring necessary SPIR-V built-ins on the fly for a while now, meaning that we don't need them to be forward-declared in headers. This PR drops includes of `__spirv_ops.hpp` so that they don't appear anymore in `core.hpp` (and some other headers). The header is not removed entirely, however, because not every built-in is known to the compiler, i.e. some of them still have to be forward-declared in the header. Most likely there are other places which can be made free of uses of the header and the header itself can probably be cleaned up agressively, but I will leave it for separate future PRs.
@@ -73,10 +73,10 @@ template <typename T, typename Properties> | |||
void prefetch_impl(T *ptr, size_t bytes, Properties properties) { | |||
#ifdef __SYCL_DEVICE_ONLY__ | |||
auto *ptrGlobalAS = | |||
reinterpret_cast<__attribute__((opencl_global)) const char *>( | |||
reinterpret_cast<__attribute__((opencl_global)) const unsigned char *>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewers: compiler only declares signed and unsigned char overloads, hence the change.
We could probably tweak the compiler to have plain char
overloads as well, but I'm not sure that it won't cause any unwanted side effects, so I went with this change for now.
unsigned char
should be a safe choice when it comes to object representation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
esimd lgtm
Can you share more on how does that work? |
@uditagarwal97, see patches from @Naghasan: #1345, #1374, #1384. |
Co-authored-by: Udit Kumar Agarwal <[email protected]>
The regression was introduced in intel#18839 and the error looks like: ``` /include/sycl/multi_ptr.hpp:397:5: error: no matching function for call to '__spirv_ocl_prefetch' 397 | __spirv_ocl_prefetch(reinterpret_cast<ptr_t>(get_decorated()), NumBytes); /include/sycl/multi_ptr.hpp:397:5: note: candidate function not viable: no known conversion from 'ptr_t' (aka 'const __global char *') to 'const __global signed char *' for 1st argument 397 | __spirv_ocl_prefetch(reinterpret_cast<ptr_t>(get_decorated()), NumBytes); ``` We previously switched to use `__spirv_ocl_prefetch` that is automatically defined by the compiler instead of having its forward-declaration in our headers. However, compiler-provided declaration does not define an overload for plain `char` for some reason. Changing that may trigger some unknown and unwanted side effects, so for the meantime the problem is worked around by using `unsigned char` overload instead which should be safe (from strict aliasing rules point of view).
This is a follow-up for intel#18839 which broke host compilation of `sycl::atomic`. The actual fix is just an addition of `#include <sycl/__spirv/spirv_ops.hpp>` to `sycl/atomic.hpp`. However, I didn't want `spirv_ops.hpp` appear again in `sycl/detail/core.hpp` and therefore I went further to make sure that `sycl/atomic.hpp` isn't used by `sycl/accessor.hpp` which prompted other changes in this PR.
Our device compiler have been capable of automagically declaring necessary SPIR-V built-ins on the fly for a while now, meaning that we don't need them to be forward-declared in headers.
This PR drops includes of
__spirv_ops.hpp
so that they don't appear anymore incore.hpp
(and some other headers).The header is not removed entirely, however, because not every built-in is known to the compiler, i.e. some of them still have to be forward-declared in the header.
Most likely there are other places which can be made free of uses of the header and the header itself can probably be cleaned up agressively, but I will leave it for separate future PRs.